Add c2pa-types + typed activeManifest and manifestStore APIs #11
Conversation
🦋 Changeset detectedLatest commit: f3626e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
emensch
force-pushed
the
types-api-updates
branch
2 times, most recently
from
0b1b924 to
80d4eaa
Compare
emensch
force-pushed
the
types-api-updates
branch
from
80d4eaa to
037163a
Compare
| "version": "0.1.1", | ||
| "types": "index.d.ts", | ||
| "scripts": { | ||
| "test": "echo \"No tests to run\" && exit 0", |
There was a problem hiding this comment.
This is kind of an annoying hangup with the monorepo system I'm using right now (NX). It expects a "test" command to be available for each published package and will error out if there's no command to run, so I've put this in place to get around that.
Frankly I'll be looking to get off of NX quite soon following this and would happily revisit this when I migrate.
| edition = "2024" | ||
|
|
||
| [dependencies] | ||
| c2pa = { version = "0.62.0", features = ["pdf", "rust_native_crypto", "json_schema"], default-features = false } |
There was a problem hiding this comment.
I'm just matching the features of the V1 SDK. I don't know if clients actively need this but we do support PDFs on Verify right now and the plan is to migrate that site to use this SDK.
|
|
||
| import type { Reader } from './types/ManifestStore.js'; | ||
|
|
||
| // Renames the auto-generated "Reader" type to the more appropriate "ManifestStore" |
There was a problem hiding this comment.
ManifestStore because you only deal with the "extracted" JSON afterwards?
There was a problem hiding this comment.
This type really is just a ManifestStore despite being called a "Reader," and if I don't rename it I think it will be confusing to consumers. I.e. "why does Reader.manifestStore() return a Reader?"
| /// Returns the asset's manifest store. | ||
| /// NOTE: at the moment, CAWG data is not decoded via this function. Use WasmReader::json() if CAWG is a requirement. | ||
| #[wasm_bindgen(js_name = manifestStore)] | ||
| pub fn manifest_store(&self) -> Result<JsValue, JsError> { |
There was a problem hiding this comment.
I see what it's doing, but what is the intend? Why is the manifest store by itself needed somewhere? (Especially because I see the js below: const manifestStore = await reader.json();, so it puzzles me a bit). Why is Reader.json() not usable all the time?
There was a problem hiding this comment.
I would like to get rid of json in favor of manifest_store only. The output is very similar but not exactly the same. Two things:
- The types generated in this PR only correspond to the output of serializing a
reader, not callingReader::json. They are close but not identical. - It looks like we can only get CAWG data through
json()right now, unless I'm mistaken. Serializing a reader in this way seems to sidestep thepost_validate_asynchook.
I'm hopeful that once @scouten-adobe is able to land contentauth/c2pa-rs#1370 and these methods return CAWG correctly, I can just get rid of json() and use these typed APIs only.
| const reader = readerMap.get(readerId); | ||
| return { data: reader.activeManifest() }; | ||
| }, | ||
| reader_json(readerId: number): WorkerResponse<string> { |
There was a problem hiding this comment.
I am confused why we need both reader_json and reader_manifestStore?
Shouldn't reader.manifestStore() be a subset of reader.json()?
There was a problem hiding this comment.
I am unconfused now - #11 (comment) is a realted great explanation.
|
@gpeacock this PR is only updating the API, a previous PR landed the "foundational" stuff, including resource store access. See that here: https://github.com/contentauth/c2pa-js-v2/pull/1/files#diff-2847b4b683a0f1f6efd121cb960f49946e99ea9c85afdd7687abfe571d538e44R58 |
Uh oh!
There was an error while loading. Please reload this page.